MSC4075 Use expirationTS to define the call ringing window #4521

BillCarsonFr · 2025-09-18T13:44:33Z

Pull Request Checklist

As per MSC4075

Notify events that are older than lifetime are ignored (using the sender_ts timestamp).
Otherwise a client syncing for the first time would ring for outdated call events. In general ringing only makes sense in "real time". Any client which is not able to receive the event in the lifetime period should not ring to prohibit annoying/misleading/irrelevant/outdated rings.

I read the contributing guide.
Pull request contains a changelog label.

UI changes have been tested with:

iPhone and iPad simulators in portrait and landscape orientations.
Dark mode enabled and disabled.
Various sizes of dynamic type.
Voiceover enabled.

codecov · 2025-09-18T14:27:35Z

Codecov Report

❌ Patch coverage is 86.02941% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.95%. Comparing base (2eaad89) to head (7ebd21a).
⚠️ Report is 122 commits behind head on develop.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...rces/Services/ElementCall/ElementCallService.swift	57.14%	15 Missing ⚠️
NSE/Sources/NotificationHandler.swift	0.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4521      +/-   ##
===========================================
- Coverage    79.16%   78.95%   -0.22%     
===========================================
  Files          836      839       +3     
  Lines        78758    79506     +748     
===========================================
+ Hits         62346    62770     +424     
- Misses       16412    16736     +324

Flag	Coverage Δ
unittests	`71.59% <86.02%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sonarqubecloud · 2025-09-19T14:33:56Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

stefanceriu · 2025-09-24T04:56:01Z

UnitTests/Sources/ElementCallServiceTests.swift

+
+        let pkPushPayloadMock = PKPushPayloadMock().addSeconds(currentDate, lifetime: 30)
+
+        service.pushRegistry(pushRegistry, didReceiveIncomingPushWith: pkPushPayloadMock, for: .voIP) {


I would've moved everything in ElementCallService.pushRegistry(didReceiveIncomingPushWith) to a separate method taking just they payload's dictionary. We don't actually need anything else and there's no reason to involve CallKit in the unit tests.

We don't actually need anything else and there's no reason to involve CallKit in the unit tests.

Given the intention to test the timeouts, I think we need to involve CallKit here to assert the timeout properly?

stefanceriu · 2025-09-24T05:00:14Z

ElementX/Sources/Services/ElementCall/ElementCallService.swift

+    init(callProvider: CXProviderProtocol? = nil, timeClock: Time? = nil) {
        pushRegistry = PKPushRegistry(queue: nil)

+        self.timeClock = timeClock ?? Time(clock: ContinuousClock(), nowDate: Date.init)


Why not make it the default value in the constructor parameter directly?

It would be nice if the struct properties have default values too. Then the constructor could be timeClock: Time = Time()

stefanceriu · 2025-09-24T05:18:20Z

project.yml

+  Clocks:
+    url: https://github.com/pointfreeco/swift-clocks
+    from: 1.0.6


I'm not a big fan of non "native" solutions for root problems and more so of code from pointfreeco. We should figure out a solution that doesn't bring in a third party component for something as core as this, even if it's our own component.

In this particular case though I don't think we need any of that:

let's have that new version of that pushRegistry(didReceiveIncomingPushWith) method expose a cancellation callback

pass in a significant but small timeout in the dictionary payload

an expectation that we .fulfill() in the callback together with waitForExpectations

I'm not a big fan of non "native" solutions for root problems and more so of code from pointfreeco. We should figure out a solution that doesn't bring in a third party component for something as core as this, even if it's our own component.

Tbh, I think it would be nice to have this kind of testable clock infrastructure. I'd be happy if we implemented it ourselves but let's leave that as an exercise for us though, once this is merged?

pixlwave

Chatted with Stefan about this a bit more. I think we're happy over all with introducing the clocks library for now and we can implement something ourselves later to avoid the dependency.

Just a few comments below 👇

NSE/Sources/NotificationHandler.swift

ElementX/Sources/Services/ElementCall/ElementCallService.swift

pixlwave · 2025-10-08T09:07:34Z

UnitTests/Sources/ElementCallServiceTests.swift

+
+        let pkPushPayloadMock = PKPushPayloadMock().addSeconds(currentDate, lifetime: 30)
+
+        service.pushRegistry(pushRegistry, didReceiveIncomingPushWith: pkPushPayloadMock, for: .voIP) {


We don't actually need anything else and there's no reason to involve CallKit in the unit tests.

Given the intention to test the timeouts, I think we need to involve CallKit here to assert the timeout properly?

ElementX/Sources/Services/ElementCall/ElementCallService.swift

pixlwave · 2025-10-08T09:13:29Z

ElementX/Sources/Services/ElementCall/ElementCallService.swift

+    init(callProvider: CXProviderProtocol? = nil, timeClock: Time? = nil) {
        pushRegistry = PKPushRegistry(queue: nil)

+        self.timeClock = timeClock ?? Time(clock: ContinuousClock(), nowDate: Date.init)


It would be nice if the struct properties have default values too. Then the constructor could be timeClock: Time = Time()

pixlwave · 2025-10-08T09:36:31Z

project.yml

+  Clocks:
+    url: https://github.com/pointfreeco/swift-clocks
+    from: 1.0.6


I'm not a big fan of non "native" solutions for root problems and more so of code from pointfreeco. We should figure out a solution that doesn't bring in a third party component for something as core as this, even if it's our own component.

Tbh, I think it would be nice to have this kind of testable clock infrastructure. I'd be happy if we implemented it ourselves but let's leave that as an exercise for us though, once this is merged?

UnitTests/Sources/ElementCallServiceTests.swift

BillCarsonFr · 2025-10-23T07:26:34Z

closed in favor of #4652 due to a lot of changes on develop

BillCarsonFr added the pr-change for updates to an existing feature label Sep 18, 2025

Base automatically changed from valere/call/subscribe_to_call_decline to develop September 18, 2025 14:11

BillCarsonFr added 3 commits September 18, 2025 22:49

Listen to call decline to stop ringing when declined from other device

2adbec5

review: revert some invisible char/tab changes

6675a85

MSC4075 Use expirationTS to define the call ringing window

6a85119

BillCarsonFr force-pushed the valere/call/notification_lifetime branch from c1e8df2 to 6a85119 Compare September 18, 2025 20:51

pixlwave and others added 3 commits September 19, 2025 09:57

Implement a simple ElementCallService test.

c92783b

Male CallService testable + tests

96edf48

swift clocks only used in tests

7ebd21a

BillCarsonFr marked this pull request as ready for review September 22, 2025 07:15

BillCarsonFr requested a review from a team as a code owner September 22, 2025 07:15

BillCarsonFr requested review from Velin92 and removed request for a team September 22, 2025 07:15

stefanceriu reviewed Sep 24, 2025

View reviewed changes

stefanceriu removed the request for review from Velin92 September 24, 2025 05:26

pixlwave reviewed Oct 8, 2025

View reviewed changes

BillCarsonFr mentioned this pull request Oct 23, 2025

Use the call expiration timestamp to define the ringing window #4652

Merged

6 tasks

BillCarsonFr closed this Oct 23, 2025


		let pkPushPayloadMock = PKPushPayloadMock().addSeconds(currentDate, lifetime: 30)

		service.pushRegistry(pushRegistry, didReceiveIncomingPushWith: pkPushPayloadMock, for: .voIP) {

MSC4075 Use expirationTS to define the call ringing window #4521

MSC4075 Use expirationTS to define the call ringing window #4521

Uh oh!

Conversation

BillCarsonFr commented Sep 18, 2025

Pull Request Checklist

Uh oh!

codecov bot commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sonarqubecloud bot commented Sep 19, 2025

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pixlwave left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BillCarsonFr commented Oct 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Sep 18, 2025 •

edited

Loading